Support splice shared input signing #4024
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
|
Also CI is sad. |
The `handle_channel_resumption` path is reachable from both channel reestablish and monitor update completion. Since we only want to sign once we know the monitor update has completed, it's possible we could have unintentionally attempted to sign if we were still pending the monitor update but had a channel reestablish occur.
This is reachable if the event doesn't get handled and a channel reestablish occurs.
wpaulino
force-pushed
the
sign-splice-shared-input
branch
from
1512cd9 to
8c9cc05
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4024 +/- ##
==========================================
- Coverage 88.77% 88.74% -0.03%
==========================================
Files 175 176 +1
Lines 127846 128638 +792
Branches 127846 128638 +792
==========================================
+ Hits 113492 114164 +672
- Misses 11788 11877 +89
- Partials 2566 2597 +31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| inputs | ||
| .iter() | ||
| .position(|input| { | ||
| input.txin.previous_output == shared_funding_input.input.previous_output |
There was a problem hiding this comment.
Shouldn't we fail or assert if we can't find the shared input?
There was a problem hiding this comment.
We just checked for it at the top of the method.
There was a problem hiding this comment.
Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.
| let mut witness = Witness::new(); | ||
| witness.push(Vec::new()); |
There was a problem hiding this comment.
Could you comment on why an empty Vec is pushed?
There was a problem hiding this comment.
There's a bug in OP_CHECKMULTISIG that pops an extra argument from the stack than what is required, so all witnesses for it need this.
There was a problem hiding this comment.
Right, but a comment in the code would be useful.
| }) | ||
| .unwrap_or(false) | ||
| { | ||
| debug_assert!(false); |
There was a problem hiding this comment.
I guess it could? I was thinking it was already covered by the state checks above, so the assert here is really to make sure we catch a mismatch in the splice state.
This commit tracks all data related to the shared input of a splice, such that a valid witness can be formed upon the splice transaction finalization.
We also remove the `Result` to make it clear that this method does not support async operations yet and rename the method to clarify that it is only intended to be used for the shared input of a splice.
wpaulino
force-pushed
the
sign-splice-shared-input
branch
from
8c9cc05 to
e664b7e
Compare
| inputs | ||
| .iter() | ||
| .position(|input| { | ||
| input.txin.previous_output == shared_funding_input.input.previous_output |
There was a problem hiding this comment.
Ah, didn't see the whole context. Was thinking we could move the shared_input_index there, but I guess we can't because sorting would make it invalid.
| let mut witness = Witness::new(); | ||
| witness.push(Vec::new()); |
There was a problem hiding this comment.
Right, but a comment in the code would be useful.
|
Merging this since the changes after @TheBlueMatt's approval are trivial. |
This also addresses follow-ups from #3889.